Return ErrBusyBuffer instead of driver.ErrBadConn #611
Conversation
|
Interesting, we were always looking for a way to reproduce those errors. In that case, we should probably return a more descriptive error like e.g. |
|
And please don't ignore our pull request template, specifically |
| if b.length > 0 { | ||
| return nil | ||
| return nil, ErrUnreadTxRows |
There was a problem hiding this comment.
Please do not return the error here. Leave the buffer unchanged.
This specific error should only be returned when we know that it is this specific error.
| return driver.ErrBadConn | ||
| data, err := mc.buf.takeSmallBuffer(pktLen + 4) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
For example here it should be impossible that we're in a transaction with unread rows. Here the driver.ErrBadConn should still be returned as there is this is a brand new connection and there really shouldn't be any unread data in the buffer.
There was a problem hiding this comment.
Had some time to sort things out, I think that's correct behaviour to return ErrBusyBuffer because when we cannot take it then something must be wrong in the code (multiple functions are using the buffer at the same time) or in this case buffer has some unread data.
And only two functions writePacket and readPacket should return ErrBadConn since they do the io.
So we should return ErrBusyBuffer anyway not ErrBadConn, and probably add additional checks to Query and Exec to return ErrUnreadRows to be more descriptive.
What do you think?
|
@amenzhinsky do you still have problems with the now implemented behavior (see #302)? |
|
Close this because no reply. |
#314
We ran into problem when using the same tx and querying multiple times, the mysql driver returns
driver.ErrBadConnsending thebusy buffererror to stderr. I believe that in most cases buffer cannot be taken only when previous one hasn't been read out or closed.See
TestUnclosedRowsas an example in the changes.This is more like error message improvement since current behaviour is quite confusing.